Skip to content

[tests] Use Android.Util.Log in JnienvTest.ThreadReuse callback#11614

Merged
jonathanpeppers merged 2 commits into
mainfrom
jonathanpeppers/tests-threadreuse-log-fix
Jun 10, 2026
Merged

[tests] Use Android.Util.Log in JnienvTest.ThreadReuse callback#11614
jonathanpeppers merged 2 commits into
mainfrom
jonathanpeppers/tests-threadreuse-log-fix

Conversation

@jonathanpeppers

Copy link
Copy Markdown
Member

Context

When the JnienvTest.ThreadReuse callback runs on a raw native pthread spawned by libreuse-threads.so and attached to the JVM, calling Console.WriteLine is unsafe: NUnit 3 replaces Console.Out with a TextCapture that resolves the current TestExecutionContext on every write. From a thread NUnit doesn''t know about, it tries to manufacture an AdhocContext and NREs in MethodWrapper.get_Name, which SIGABRTs the entire process and causes the test host to report "0 tests ran" for the whole assembly:

E DOTNET: Unhandled exception. System.NullReferenceException
  at NUnit.Framework.Internal.MethodWrapper.get_Name()
  at NUnit.Framework.Internal.TestExecutionContext.AdhocContext..ctor()
  at NUnit.Framework.Internal.Execution.TextCapture.WriteLine(String)
  at System.Console.WriteLine(...)
  at Java.InteropTests.JnienvTest.<>c.<ThreadReuse>b__7_0(IntPtr, IntPtr)
F libc: Fatal signal 6 (SIGABRT) in tid 5229 (Thread-14)

Fix

Route the diagnostics through Android.Util.Log.Info to bypass NUnit''s Console.Out redirect entirely. No try/catch is needed — the Console -> Log conversion is the actual fix.

Split out of #11224 as a small, standalone change so it can land independently.

Build 14271469 / APKs 1 / Mono.Android.NET_Tests-Release reproduced
the exact NRE first diagnosed in 09acb6d:

  E DOTNET: Unhandled exception. System.NullReferenceException
    at NUnit.Framework.Internal.MethodWrapper.get_Name()
    at NUnit.Framework.Internal.TestExecutionContext.AdhocContext..ctor()
    at NUnit.Framework.Internal.Execution.TextCapture.WriteLine(String)
    at System.Console.WriteLine(...)
    at Java.InteropTests.JnienvTest.<>c.<ThreadReuse>b__7_0(IntPtr, IntPtr)
  F libc: Fatal signal 6 (SIGABRT) in tid 5229 (Thread-14)

The 'minimize the diff' commit 4d924c4 reverted both the Console-to-Log
conversion and the try/catch -- but only the try/catch needed to go.
NUnit 3's Console.Out redirection is unsafe on a raw native pthread
that has no TestExecutionContext, so route the diagnostics through
Android.Util.Log to bypass the redirect entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 9, 2026 19:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the JnienvTest.ThreadReuse diagnostic output to avoid using Console.WriteLine from a raw native pthread callback (spawned by libreuse-threads.so), which can crash under NUnit 3 due to Console.Out redirection resolving TestExecutionContext on unknown threads.

Changes:

  • Replace Console.WriteLine diagnostics inside the native-thread callback with Android.Util.Log.Info.
  • Add an in-code note explaining why Console.WriteLine is unsafe in this scenario.

@jonathanpeppers jonathanpeppers added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jun 10, 2026
@jonathanpeppers jonathanpeppers merged commit c356b60 into main Jun 10, 2026
40 checks passed
@jonathanpeppers jonathanpeppers deleted the jonathanpeppers/tests-threadreuse-log-fix branch June 10, 2026 14:06
jonathanpeppers added a commit that referenced this pull request Jun 10, 2026
## Summary

Migrate Mono.Android device test infrastructure from the legacy NUnitLite stack to `dotnet test` with the Microsoft Testing Platform (MTP), following the pattern established for the `androidtest` template in 031246d.

Callers now run device tests with stock `dotnet test`, no custom MSBuild targets required. **NUnitLite is removed entirely** from the workload — both as a framework assembly and as a vendored copy.

## Changes

### New: MTP-based `TestInstrumentation` base class

- `tests/TestRunner.Core/TestInstrumentation.cs` is now an MTP-based on-device runner using stock NUnit (`NUnitTestAssemblyRunner`) plus `Microsoft.Testing.Platform`.
- Mirrors the `androidtest` template: tests run synchronously on the instrumentation thread (`NumberOfTestWorkers = 0`, see comment in code), TRX is written to the app data dir, and the path is sent back through the instrumentation bundle so the host adapter can pick it up.

### Updated test projects

- **Mono.Android.NET-Tests**, **Java.Interop-Tests**, **JcwGen-Tests**: now reference stock NUnit `3.13.3` from NuGet (no NUnitLite / TestRunner.NUnit), set `IsTestingPlatformApplication=true`, and use the new `TestInstrumentation` base.

### Deleted (~41K lines)

- `src/Xamarin.Android.NUnitLite/` — the `Xamarin.Android.NUnitLite.dll` framework assembly (NUnitLite wrapper, `TestSuiteInstrumentation`, GUI `TestSuiteActivity` / `TestResultActivity` / `OptionsActivity`, layouts).
- `src-ThirdParty/NUnitLite/` — the embedded NUnitLite source we used to compile into the above.
- `tests/TestRunner.NUnit/` — `NUnitTestRunner` / `NUnitTestInstrumentation`.
- `tests/TestRunner.xUnit/` — `XUnitTestRunner` / `XUnitTestInstrumentation`.
- `Xamarin.Android.NUnitLite.dll/.pdb/.xml` removed from `build-tools/installers/create-installers.targets` (no longer shipped in the framework).
- Various custom `RunTestApp` / device-test MSBuild targets.

The `XA4313` deprecation message in `Common.targets` (which suggested customers migrate `Xamarin.Android.NUnitLite` → `Xamarin.Legacy.NUnitLite`) is preserved as the existing escape hatch; the `Xamarin.Legacy.NUnitLite` NuGet is published from a separate repo and is unaffected by these changes.

### NativeAOT trimmer fix

NUnit 3.13.3's `EquatablesComparer` walks every value via reflection: `type.GetInterfaceMap(typeof(IEquatable<T>))` then `MethodInfo.Invoke` on the result. Under stock NativeAOT this throws `Arg_NotSupportedException` because ILC strips the dispatch-slot mapping. NUnit 3.14+ guards this with try/catch, but we cannot bump globally (it regresses the Mono Interpreter `AwaitAdapter` lane).

Real fix, no `[Category("NativeAOTIgnore")]` shortcuts:

- `tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj`: under `'$(PublishAot)' == 'true'`, set `<IlcGenerateCompleteTypeMetadata>true</IlcGenerateCompleteTypeMetadata>` and pull in a new `NativeAOT.rd.xml`.
- `tests/Mono.Android-Tests/Mono.Android-Tests/NativeAOT.rd.xml` roots the closed `IEquatable<T>` instantiations the test value types need (`String`, `Boolean`, `IntPtr`, `TimeSpan`, `DateTime`) plus the concrete types themselves as `Dynamic="Required All"`, so ILC keeps the slot mapping. Also roots `TaskAwaitAdapter+GenericAdapter<VoidTaskResult>` for NUnit's async adapter.

Result on CI build [14325032](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=14325032): NativeAOT lane 230 passed / 0 failed (43 legitimate category exclusions).

### CI updates (`apk-instrumentation.yaml`)

- Build with `-t:Install`, run with `dotnet test --no-build --report-trx`.
- Test results are published as VSTest (TRX) instead of NUnit XML.
- Added `adb logcat -d` capture so device-test crashes can be diagnosed without re-running CI.

### Test fixes / bookkeeping

- Centralized `JavaSystem.LoadLibrary ("reuse-threads")` in `TestInstrumentation.OnCreate` so it runs on the main Java thread instead of inside individual tests.
- Synced `ExcludedTestNames` / `ExcludedCategories` to match `main`.

## Out of scope

- `EmbeddedDSO` and `Locale-Tests` — old-style csproj that still `<Reference Include="Xamarin.Android.NUnitLite" />`. They need an SDK migration before this PR can fully delete that assembly's identity from the build (the `XA4313` deprecation will fire on these projects today). Tracked separately.

## Already split out

These improvements were carved out of this PR and merged separately to `main`:

- #11614 — `[tests] Use Android.Util.Log in JnienvTest.ThreadReuse callback`
- #11615 — `[tests] Improve DoNotLeakWeakReferences reliability`
- #11609 — `Bump to dotnet/java-interop@b881d21` — picks up dotnet/java-interop#1437 (modernize NUnit attributes), which is why no `NUnitCompatibility.cs` shim is needed in this PR.

## Related

- Template work: 031246d
- #11523 — fixed `UrlEscaping_Bug43411` on `main` (test had been silently dead under NUnitLite)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants